-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add search box props for clear button aria attributes #3946
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add search box props for clear button aria attributes #3946
Conversation
…ion for clear button
| /** | ||
| * The aria description of the clear button for the SearchBox for the benefit of screen readers. | ||
| */ | ||
| clearButtonAriaDescription?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
we believe that these props be a part of the clearButtonProps property bag. That would make it easier to evolve the APIs.
clearButtonProps: {
ariaLabel;
ariaDescription:
} #Resolved
manishgarg1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕐
| * The aria label of the clear button for the SearchBox for the benefit of screen readers. | ||
| */ | ||
| clearButtonAriaLabel?: string; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to remove this. right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
…c-react into bugs/3858/clear-search-arialabel-ariadescription
| /** | ||
| * The props for the clear button. | ||
| */ | ||
| clearButtonProps?: ISearchBoxClearButtonProps; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should just be IButtonProps which should support whatever a button supports. That allows the caller to do things like put a data-automation-id on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd considered that but thought I'd start with the conservative approach of whitelisting properties. Is this the last blocking issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify: I will change this to ibuttonprops tomorrow, I'm just asking if there are other changes needed as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
…c-react into bugs/3858/clear-search-arialabel-ariadescription
|
Excellent, thanks @manishgarg1. What's the best way for us to know when changes like this and #3961 (if it is merged) are shipped? Is there a regular release schedule or some release tags or something to check? |
|
@hoovercj I think releases are automated and come out roughly once a day. You can view the release log to see the latest, with notes on what's new. |
|
Oh excellent, exactly what I was looking for. Thanks @mikewheaton |
Pull request checklist
$ npm run changeDescription of changes
Add clearButtonAriaLabel and clearButtonAriaDescription props to SearchBox.types.ts and pass them to the icon button in SearchBox.base.tsx
Focus areas to test
Ensure that passing the props result in the props being applied to the icon button and read to screen readers when the button has focus.